Fix fcoalesce Date/IDate mixing and resolve test collisions (#7463)#7557
Open
aksh08022006 wants to merge 4 commits intoRdatatable:masterfrom
Open
Fix fcoalesce Date/IDate mixing and resolve test collisions (#7463)#7557aksh08022006 wants to merge 4 commits intoRdatatable:masterfrom
aksh08022006 wants to merge 4 commits intoRdatatable:masterfrom
Conversation
ben-schwen
reviewed
Jan 3, 2026
| if (TYPEOF(x)!=VECSXP) internal_error(__func__, "input is list(...) at R level"); // # nocov | ||
| if (!IS_TRUE_OR_FALSE(inplaceArg)) internal_error(__func__, "argument 'inplaceArg' must be TRUE or FALSE"); // # nocov | ||
| if (!IS_TRUE_OR_FALSE(nan_is_na_arg)) internal_error(__func__, "argument 'nan_is_na_arg' must be TRUE or FALSE"); // # nocov | ||
| if (TYPEOF(x) != VECSXP) |
Member
There was a problem hiding this comment.
please dont reformat existing code. This makes reviewing quite tedious.
ben-schwen
reviewed
Jan 3, 2026
| VDate = fifelse(VDate == as.Date("3000-01-01"), as.Date(NA), VDate), | ||
| VIDate = fifelse(VIDate == as.IDate("3000-01-01"), as.IDate(NA), VIDate), | ||
| VNumA, VNumB, Count, Y_Sum)]) | ||
|
|
Member
There was a problem hiding this comment.
Please no formatting changes.
ben-schwen
reviewed
Jan 3, 2026
|
|
||
| # fcoalesce Date and IDate mixing, #7463 | ||
| test(2357.1, fcoalesce(as.Date("2026-01-01"), as.IDate("2026-01-02")), as.Date("2026-01-01")) | ||
| test(2357.2, { |
Member
There was a problem hiding this comment.
please dont make tests super complicated. {} should only be used when you are confident that it makes the test easier to read and you necessarily need multiple state. this is probably not warranted.
Member
|
I can review, once the formatting diffs are removed. Note that we already have a similar promoting logic in place for |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR addresses #7463 by allowing fcoalesce() and setcoalesce() to handle mixing of Date (double) and IDate (integer) classes, as well as general mixing of integer and double types. Previously, these combinations would throw a strict type/class mismatch error.
Technical Details The core of the change is in
src/coalesce.c
. I've updated the logic to:
Relax Type Checks: Added a promote_to_real flag that triggers when fcoalesce sees an integer vector followed by a double. This allows fcoalesce to return a REALSXP vector even if the first argument was INTSXP.
Handle Date/IDate Inheritance: Modified the class-matching check to allow any two objects that both inherit from the Date class. This handles the specific pairing of Date and IDate which were previously rejected as "different classes".
Strict In-place Rules: For setcoalesce(), I've kept the strict rule that we cannot promote an integer vector to double in-place (as that would require memory reallocation). It now throws a specific, helpful error suggesting manual coercion instead of a generic "type mismatch".
Attribute Persistence: Corrected the attribute handling so that if we promote an IDate to a double Date, the class attributes are correctly propagated to the new object.
Testing I've added six new tests to the end of
inst/tests/tests.Rraw
(numbered 2357.1–2357.6) covering:
fcoalesce(Date, IDate)
fcoalesce(IDate, Date) (promotion check)
Numeric mixing (INTSXP + REALSXP)
setcoalesce safety checks (ensuring we don't accidentally reallocate in-place).
Verified that these fail on the current master and pass with this build. I also re-ran the full local test suite to ensure no regressions in existing fcoalesce logic.
A Note to Maintainers I've tried my best to follow the data.table coding style and memory safety principles, but I'm still getting my head around some of the C-level internals. I could be wrong about how I've handled the promotion flags or if there's a more efficient way to check for the Date class inheritance at this level. I'm very much open to learning if there's a better or more "the data.table way" to implement this!